Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Perform download safety check even when user is prompted for save location. #16969

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Perform download safety check even when user is prompted for save location. #16969

merged 1 commit into from
Feb 3, 2023

Conversation

mkarolin
Copy link
Collaborator

@mkarolin mkarolin commented Feb 1, 2023

Resolves brave/brave-browser#28079

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@mkarolin mkarolin self-assigned this Feb 1, 2023
@mkarolin mkarolin requested review from a team as code owners February 1, 2023 22:50
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

⚠️ PR head is an unsigned commit
commit: 19a22e9c98c51abd89cc5a16f2d027a0cddb3378
reason: unsigned
Please follow the handbook to configure commit signing
cc: @mkarolin

@mkarolin mkarolin merged commit 83021ed into master Feb 3, 2023
@mkarolin mkarolin deleted the maxk-download-safety-check branch February 3, 2023 20:24
@mkarolin mkarolin added this to the 1.50.x - Nightly milestone Feb 3, 2023
brave-builds added a commit that referenced this pull request Feb 8, 2023
@brave-builds
Uplift of #16969 (squashed) to beta
f36fd25
@brave-builds brave-builds mentioned this pull request Feb 8, 2023
Merged
7 tasks
brave-builds added a commit that referenced this pull request Feb 8, 2023
@brave-builds
Uplift of #16969 (squashed) to release
9304630
@brave-builds brave-builds mentioned this pull request Feb 8, 2023
Merged
7 tasks
@kjozwiak
Copy link
Member

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.50.39 Chromium: 110.0.5481.100 (Official Build) nightly (64-bit)
-- | --
Revision | 4be7a36f7cb943af6118e449bbab494b43dcaddd-refs/branch-heads/5481_77@{#14}
OS | Windows 11 Version 22H2 (Build 22621.1265)

Using the STR/Cases outlined via brave/brave-browser#28079 (comment), went through the following:

Test Case #1 - Brave Default (Ask where to save each file before downloading enabled)

  • ensured that Ask where to save each file before downloading was enabled via brave://settings/downloads
  • ensured that the Windows Save As prompt/modal appeared when opening/loading scfExtension.html
  • ensured that the This type of file can harm your computer error appeared at the bottom of the window
    • ensured that selecting Keep saved the @aexample.scf into the correct location
    • ensured that selecting Discard removes the file and doesn't save it (removes entry from brave://downloads as well)
    • ensured that clicking on Show all opens brave://downloads and lists the current entry with the warning
  • ensured the entry within brave://downloads has the option to Discard or Keep the file
    • ensured that selecting Discard removes the file and doesn't save it (removes entry from brave://downloads as well)
    • ensured that selecting Keep displayed a modal confirmation about saving the file locally
      • ensured that selecting Cancel returns the user back to brave://downloads but doesn't remove the file/decision
      • ensured that selecting Keep downloads the @aexample.scf file locally without any issues
    • ensured that the @aexample.scf entry appears within brave://downloads if Keep was selected
Example Example Example Example Example
download0 download1 download2 download3 download4

Test Case #2 - Brave Custom (Ask where to save each file before downloading disabled)

  • disable Ask where to save each file before downloading via brave://settings/downloads
  • ensured that the This type of file can harm your computer error appeared at the bottom of the window
    • ensured that selecting Keep saved the @aexample.scf into the correct location
    • ensured that selecting Discard removes the file and doesn't save it (removes entry from brave://downloads as well)
    • ensured that clicking on Show all opens brave://downloads and lists the current entry with the warning
  • ensured the entry within brave://downloads has the option to Discard or Keep the file
    • ensured that selecting Discard removes the file and doesn't save it (removes entry from brave://downloads as well)
    • ensured that selecting Keep displayed a modal confirmation about saving the file locally
      • ensured that selecting Cancel returns the user back to brave://downloads but doesn't remove the file/decision
      • ensured that selecting Keep downloads the @aexample.scf file locally without any issues
    • ensured that the @aexample.scf entry appears within brave://downloads if Keep was selected
Example Example Example
ask1 ask2 ask3

Test Case #3 - Installing Packed Extension

Using the STR/Cases outlined via brave/brave-browser#28079 (comment), packed the 1Password extension and ensured that it installed without any warnings re: the files not being safe as per the following:

Example Example Example Example
extension1 extension2 extension3 extension4

kjozwiak pushed a commit that referenced this pull request Feb 16, 2023
@brave-builds
Perform download safety check even when user is prompted for save loc…
d119b46 
…ation. (uplift to 1.49.x) (#17112)

Uplift of #16969 (squashed) to beta
kjozwiak pushed a commit that referenced this pull request Feb 16, 2023
@brave-builds
Perform download safety check even when user is prompted for save loc…
532baf7 
…ation. (uplift to 1.48.x) (#17113)

Uplift of #16969 (squashed) to release
@kjozwiak kjozwiak mentioned this pull request Feb 16, 2023
Closed
bridiver
bridiver reviewed Mar 9, 2023
View reviewed changes
chromium_src/chrome/browser/download/download_target_determiner.cc
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

// Prompting the user for download location shouldn't be a factor in determining
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused because this looks like an upstream issue if there is an issue here, but the location does affect the danger level which is why macOS prompts for permission to access different file system locations for each app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver left review comments

@goodov goodov goodov approved these changes

@diracdeltas diracdeltas diracdeltas approved these changes

@fmarier fmarier fmarier approved these changes

Assignees

@mkarolin mkarolin

Labels
None yet
Projects
None yet
Milestone
1.50.x - Release
Development

Successfully merging this pull request may close these issues.

[hackerone] always show both file dialogs on windows
6 participants
@mkarolin @kjozwiak @bridiver @fmarier @diracdeltas @goodov